-
Notifications
You must be signed in to change notification settings - Fork 223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds 'faas-cli template pull' command #201
Conversation
7f00b90
to
b83a368
Compare
@ericstoekl
|
b83a368
to
1e80b83
Compare
@ericstoekl perhaps we should not make languages available where there is no template.yml file in the directory. |
I've tested the PR and really liked the experience, this will be a great addition to our CLI. For follow-up work post-merge, let's talk about:
|
commands/fetch_templates.go
Outdated
if languageSplit := strings.Split(relativePath, "/"); len(languageSplit) > 2 { | ||
language = languageSplit[1] | ||
|
||
if len(languageSplit) == 3 && relativePath[len(relativePath)-1:] == "/" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-squash I asked for these magic numbers to be documented / split out either into a new function or named consts. If I had to work on this I wouldn't know what to do.
commands/fetch_templates.go
Outdated
@@ -137,3 +186,29 @@ func createPath(relativePath string, perms os.FileMode) error { | |||
err := os.MkdirAll(dir, perms) | |||
return err | |||
} | |||
|
|||
// Tells whether the language can be expanded from the zip or not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment should start "canWriteLanguage tells whether"
commands/fetch_templates.go
Outdated
func canWriteLanguage(availableLanguages *map[string]bool, language string, overwrite bool) bool { | ||
canWrite := false | ||
if len(language) > 0 { | ||
if _, ok := (*availableLanguages)[language]; ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add if availableLanguages != nil
and change ok
to exists
or found
commands/fetch_templates_test.go
Outdated
} | ||
|
||
func Test_fetchTemplates(t *testing.T) { | ||
defer tearDown_fetch_templates(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be run at the start and at the end - because you may have had an inconsistent file-system if the files were written out and the code panicked and crashed mid-way.
@@ -44,15 +44,23 @@ language or type in --list for a list of languages available.`, | |||
} | |||
|
|||
func runNewFunction(cmd *cobra.Command, args []string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're changing the commands to return (error) so we can handle the exit codes during CI / scripting. If that's beyond scope of the PR please raise an issue instead to log the work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take you up on the second option: #210
commands/new_function.go
Outdated
found = true | ||
func printAvailableTemplates(availableTemplates []string) string { | ||
var result string | ||
for _, template := range availableTemplates { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we sort "printAvailableTemplates" deterministically?
commands/template_pull.go
Outdated
@@ -0,0 +1,80 @@ | |||
// Copyright (c) Alex Ellis, Eric Stoekl 2017. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
License should be:
// Copyright (c) Alex Ellis 2017. All rights reserved.
or
// Copyright (c) OpenFaaS Project 2017. All rights reserved.
Please see: https://github.com/openfaas/faas/blob/master/CONTRIBUTING.md#license
commands/template_pull_test.go
Outdated
@@ -0,0 +1,144 @@ | |||
// Copyright (c) Alex Ellis, Eric Stoekl 2017. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,56 @@ | |||
# Using template from external repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be in the guides in the faas repo?
stack/language_template.go
Outdated
return ParseYAMLDataForLanguageTemplate(fileData) | ||
} | ||
|
||
// iParseYAMLDataForLanguageTemplate parses YAML data into language template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo of "i" at start of comment.
stack/schema.go
Outdated
@@ -47,3 +47,8 @@ type Services struct { | |||
Functions map[string]Function `yaml:"functions,omitempty"` | |||
Provider Provider `yaml:"provider,omitempty"` | |||
} | |||
|
|||
type LanguageTemplate struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment:
// LanguageTemplate read from template.yml within root of a language template folder
commands/deploy.go
Outdated
@@ -160,6 +160,28 @@ func runDeploy(cmd *cobra.Command, args []string) { | |||
log.Fatalln(envErr) | |||
} | |||
|
|||
// Get FProcess to use from the ./template/template.yml, if a template is being used | |||
if function.Language != "" && function.Language != "Dockerfile" && function.Language != "dockerfile" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract to named method.
Change to this or similar:
len(function.Language) > 0 && strings.Lower(function.Language) != "dockerfile"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very close to finished. Please address comments and licensing then add a new commit, do not squash.
1) Moves code that merges zip file contents into template/ dir into expandTemplatesFromZip() function. Removes hard-coded constant. 2) Sort list of languages found with 'faas-cli new --list' before printing 3) Fixes for typos, more descriptive comments, and remove incorrect license attribution Signed-off-by: Eric Stoekl <ems5311@gmail.com>
Hi @alexellis , thanks for your comments. I've attempted to address all of them except the comment about the guide needing to go into the I've created a new function in I've added sort functionality to the |
@alexellis , I have pushed another commit to address this comment: #201 (comment) This is to disallow creation of a function when If we want to split this off into a future PR, I'm okay with that as well. |
stack/language_template_test.go
Outdated
}, | ||
} | ||
|
||
for k, i := range dataProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what dataProvider
means.
@@ -0,0 +1,2 @@ | |||
language: node-armhf | |||
fprocess: node index.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the node template contain a full template or does it still only have the Dockerfile patch? cc @rgee0
commands/fetch_templates.go
Outdated
} | ||
} | ||
} else { | ||
// template/ | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the half-dozen continue
statements, how can we re-write this to be clearer?
commands/fetch_templates.go
Outdated
continue | ||
} | ||
|
||
if pathSplit := strings.Split(relativePath, "/"); len(pathSplit) > 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this all needs to be a function of its own. Extract a new function
commands/fetch_templates.go
Outdated
} | ||
|
||
// Takes a language input (e.g. "node"), tells whether or not it is OK to download | ||
func checkLanguage(language string, overwrite bool) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
templateFolderExists
/ targetExists
?
commands/fetch_templates.go
Outdated
isDirectory = relativePath[len(relativePath)-1:] == "/" | ||
|
||
// Check if this is the root directory for a language (at ./template/lang) | ||
if len(pathSplit) == rootLanguageDirSplitCount && isDirectory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are we doing here exactly?
We unzip and then don't overwrite if a template/node
folder exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, don't overwrite if the template/<lang>
folder already exists. Unless --overwrite
flag is active.
Pull language templates from any repo that has a 'template/' directory in the root with 'faas-cli template pull <github repo>'. Also allows 'faas-cli new --lang' to list available languages Signed-off-by: Eric Stoekl <ems5311@gmail.com> Signed-off-by: Minh-Quan TRAN <account@itscaro.me>
1) Moves code that merges zip file contents into template/ dir into expandTemplatesFromZip() function. Removes hard-coded constant. 2) Sort list of languages found with 'faas-cli new --list' before printing 3) Fixes for typos, more descriptive comments, and remove incorrect license attribution Signed-off-by: Eric Stoekl <ems5311@gmail.com>
is not present in language template directory Needs to add a 'template.yml' to each fake language template dir in commands/testdata/new_function/template/<lang> in order to have current tests pass. Deletes some unnecessary .gitignore files as a result. Signed-off-by: Eric Stoekl <ems5311@gmail.com>
1) Improve readability by adding canExpandTemplateData() function, which returns an enum telling what to do with the data found in the zip archive. 2) Add function data to ARMHF directories in template/ dir 3) More descriptive naming for test array in language_template_test.go and checkLanguage() function changed to templateFolderExists() Signed-off-by: Eric Stoekl <ems5311@gmail.com>
03c1bf4
to
927400a
Compare
@alexellis , just made those requested updates. I've refactored the part in the loop with |
Needs a rebase. |
1) Moves code that merges zip file contents into template/ dir into expandTemplatesFromZip() function. Removes hard-coded constant. 2) Sort list of languages found with 'faas-cli new --list' before printing 3) Fixes for typos, more descriptive comments, and remove incorrect license attribution Signed-off-by: Eric Stoekl <ems5311@gmail.com>
Rebasing and carrying out some review / clean-up over in #220. |
1) Moves code that merges zip file contents into template/ dir into expandTemplatesFromZip() function. Removes hard-coded constant. 2) Sort list of languages found with 'faas-cli new --list' before printing 3) Fixes for typos, more descriptive comments, and remove incorrect license attribution Signed-off-by: Eric Stoekl <ems5311@gmail.com>
Pull language templates from any repo that has a 'template/' directory
in the root with 'faas-cli template pull '.
Also allows 'faas-cli new --list' to list available languages
Signed-off-by: Eric Stoekl ems5311@gmail.com
Signed-off-by: Minh-Quan TRAN account@itscaro.me
Description
This PR is to add the ability to use
faas-cli template pull <github repo>
. For example:Check this asciicinema to see it in action: https://asciinema.org/a/ieyznKidFlvimpeiS5BFMG3ep
Here's a breakdown of what was changed:
Main Files
builder/build.go
- UsesIsValidTemplate()
to tell whether or not it can build the given languagecommands/build.go
- Update to passoverwrite
parameter tofetchTemplates()
commands/deploy.go
- Get Fprocess from thetemplate/<lang>/template.yml
file instead of hard-codedcommands/fetch_templates.go
-fetchTemplates()
method pulls fromopenfaas/faas-cli
github repo by default, custom repo URL can be passed in. Implements--overwrite
functionalitycommands/new_function.go
- Lists available languages from the template dir with--list
optioncommands/template_pull.go
- Make sure the passed-in valid, then passes it tofetchTemplates()
stack/language_template.go
- AddsParseYAMLForLanguageTemplate()
, which will parse thetemplate/<lang>/template.yml
file, which contains info like Fprocessstack/schema.go
- AddsLanguageTemplate
typeguide/TEMPLATE.md
- Adds a guide for usage offaas-cli template pull
template/csharp/template.yml
template/go-armhf/template.yml
template/go/template.yml
template/node-arm64/template.yml
template/node-armhf/template.yml
template/node/template.yml
template/python-armhf/template.yml
template/python/template.yml
template/python3/template.yml
template/ruby/template.yml
The rest of the files are for testing, and are discussed below.
Motivation and Context
First discussed in issue Proposal: third-party templates via GitHub #85, originally work was done in Create
faas-cli add-template --url <URL>
command #87.How Has This Been Tested?
In addition to the following files being added for testing, @burtonr reported success when manually testing: #87 (comment)
commands/fetch_templates_test.go
- Simplify tests, add test forfetchTemplates()
commands/new_function_test.go
- Test--list
option, and add more tests fornew
commands/template_pull_test.go
- Testtemplate pull
functionalitycommands/testdata/master_test.zip
- Minimal template zip for testing, served by the mock test servercommands/testdata/new_function/template/csharp/function/.gitignore
- Add directoriescommands/testdata/new_function/template/go-armhf/.gitignore
commands/testdata/new_function/template/go/function/.gitignore
commands/testdata/new_function/template/node-arm64/function/.gitignore
commands/testdata/new_function/template/node-armhf/.gitignore
commands/testdata/new_function/template/node/function/.gitignore
commands/testdata/new_function/template/python-armhf/.gitignore
commands/testdata/new_function/template/python/function/.gitignore
commands/testdata/new_function/template/python3/function/.gitignore
commands/testdata/new_function/template/ruby/function/.gitignore
stack/language_template_test.go
- TestParseYAMLForLanguageTemplate()
against different types of YAML fileTypes of changes
Checklist:
git commit -s